Add support for display archived workflow on web ui from s3 archival store #5117
Conversation
|
|
|
Got something wrong with unit test,I'll find some time to fix it. |
Aoao54
force-pushed
the
support-display-archived-workflow-on-ui
branch
from
33d46ed
to
c6d8e5a
Compare
Aoao54
force-pushed
the
support-display-archived-workflow-on-ui
branch
from
c6d8e5a
to
140790b
Compare
Aoao54
force-pushed
the
support-display-archived-workflow-on-ui
branch
from
140790b
to
dba6eb1
Compare
| for _, batch := range resp.HistoryBatches { | ||
| history.Events = append(history.Events, batch.Events...) | ||
| } | ||
| // reverse the events |
There was a problem hiding this comment.
I am a bit confused here. Looks like this is only reversing events within a page. When there are multiple pages, the returned history will look like something like:
event 100, 99, 98, ..., 1, event 200, 199, 198, ..., 101.
There was a problem hiding this comment.
Yep.There will be a mistake. Mark it on my todo list.
There was a problem hiding this comment.
I am not sure how you plan to resolve this though. It seems like a n^2 alg. if the underlying storage doesn't support reading backward.
There was a problem hiding this comment.
Not sure whether if the underlying storage support it. I'll check it out. If not, maybe there's no smarter way.
I think I'll focus on this and learning the internals of mutable state for now. Since the other changes require more design discussion。 ：）
| return true | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| func (wh *WorkflowHandler) describeArchivedWorkflowExecution(ctx context.Context, request *workflowservice.DescribeWorkflowExecutionRequest) (*workflowservice.DescribeWorkflowExecutionResponse, error) { |
There was a problem hiding this comment.
I don't think archived workflow can be described based on visibility records. Fields like pending activities or pending children can't be populated by vis records.
If we really want to do it, the right way should be rebuild workflow mutable state from history events, then describe the rebuilt mutable states so that all fields in the describe response can be populated.
Alternatively, a separate API can be created for describing archived workflow and defined a new response that returns only limited information.
cc @yiminc
There was a problem hiding this comment.
I don't think archived workflow can be described based on visibility records. Fields like pending activities or pending children can't be populated by vis records.
It seems a little weird if a archived workflow still has these pending things. Should we consider a archived wf is totally completed(without any changeable things)?
rebuild workflow mutable state from history events, then describe the rebuilt mutable states so that all fields in the describe response can be populated.
I'm not familiar with the internals of mutable state.. According to the previous experience issue, an archived workflow which has past the retention is considered deleted from mutable states. If we want to describe it based on mutable state, there maybe a lot of change to do.
A separate API is good. We need more discuss about the limited information of an archived wf and how to get it..
There was a problem hiding this comment.
workflow can still have pending things even after closed, for example when workflow is force terminated before activity finishes. Archived or not is a concept of where the data is stored, so they are orthogonal.
there maybe a lot of change to do.
Yes it's a lot of work and change to rehydrate archived workflow back into the system. But, to me, that seems the right thing to do, and not causing more confusion to the caller of DescribeWorkflowExecution.
There was a problem hiding this comment.
@yiminc Do we have any established process for external contribution, especially for changes that require some design discussion? Maybe a PR to https://github.com/temporalio/proposals is the first step?
| return true | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| func (wh *WorkflowHandler) describeArchivedWorkflowExecution(ctx context.Context, request *workflowservice.DescribeWorkflowExecutionRequest) (*workflowservice.DescribeWorkflowExecutionResponse, error) { |
There was a problem hiding this comment.
workflow can still have pending things even after closed, for example when workflow is force terminated before activity finishes. Archived or not is a concept of where the data is stored, so they are orthogonal.
there maybe a lot of change to do.
Yes it's a lot of work and change to rehydrate archived workflow back into the system. But, to me, that seems the right thing to do, and not causing more confusion to the caller of DescribeWorkflowExecution.
| for _, batch := range resp.HistoryBatches { | ||
| history.Events = append(history.Events, batch.Events...) | ||
| } | ||
| // reverse the events |
There was a problem hiding this comment.
I am not sure how you plan to resolve this though. It seems like a n^2 alg. if the underlying storage doesn't support reading backward.
|
Related issue on UI repo: temporalio/ui#1174 |
|
Why is changes required to temporal? The cli can show archived workflow executions and the web-ui supported this in the past. |
|
Hi @jontro , you can check this issues for more context. In a word , the archived WF which has past the retention can't displayed on the web ui. Because the web ui read archived WF info from history store but it will be deleted after the retention. So there is a 404 error. |
Yes but shouldn't that change be made in the web ui instead? The api already supports getting archived workflows. |
What changed?
Add index workflowRunId to s3 archival store and support query on it.
Alter DescribeWorkflowExecution func to support describeArchivedWorkflowExecution.
Why?
Support for display archived workflow on web ui. Up to now, for the archived workflow which has past the retention can't displayed on the web ui.
How did you test it?
Add an unit test TestDescribeWorkflowExecution_ArchivedStatus .
And test the feature locally by opening archived wf which has past the retention on the web ui.
Potential risks
Is hotfix candidate?